[BugFix] Fix json_set crash and json_delete no-op with $.key paths (#5167) by qianheng-aws · Pull Request #5339 · opensearch-project/sql

qianheng-aws · 2026-04-13T07:22:22Z

Description

json_set crashes (HTTP 500) and json_delete silently returns unchanged JSON when using standard JSONPath $.key syntax. The root cause is convertToJsonPath() in JsonUtils.java, which unconditionally prepends $. to the input — turning $.name into $.$.name (double-prefixed).

The fix strips any existing $. or $ prefix from the input before processing, preventing double-prefixing while preserving the existing curly-brace-to-bracket conversion logic.

Related Issues

Resolves #5167, #5170

Check List

…pensearch-project#5167) convertToJsonPath() unconditionally prepends "$." to input paths. When users pass standard JSONPath syntax like "$.name", the path becomes "$.$.name" (double-prefixed), causing json_set to crash with HTTP 500 and json_delete to silently return unchanged JSON. Strip any existing "$." or "$" prefix before processing to prevent double-prefixing while preserving the curly-brace-to-bracket conversion. Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws · 2026-04-13T07:22:39Z

Decision Log

Root Cause: JsonUtils.convertToJsonPath() unconditionally prepends $. to its input. When users pass standard JSONPath syntax like $.name, the result is $.$.name (double-prefixed). For json_set, Calcite's JsonFunctions.jsonSet interprets the invalid $. segment as a key and crashes. For json_delete, JsonFunctions.jsonRemove finds no match for $.$.name and returns the JSON unchanged.

Approach: Strip any existing $. or $ prefix from the input at the start of convertToJsonPath() before the function proceeds with its normal processing. This is the minimal change that handles all prefix variants ($, $., or none) and preserves the existing curly-brace-to-bracket conversion logic (e.g., {}→[*]).

Alternatives Rejected:

Return early if input starts with $: This would skip the curly-brace conversion, breaking hypothetical inputs like $.a{}.b that combine both syntaxes. The strip-then-process approach is more robust.
Regex-based normalization: More complex for no additional benefit; the simple prefix check is clearer and handles all cases.

Pitfalls: None encountered — the fix is localized to one utility method. All existing tests (unit + integration + YAML) continue to pass.

Things to Watch: The json_extract, json_append, and json_extend functions also call convertToJsonPath, so they all benefit from this fix. Users can now use either $.name or name interchangeably for path arguments in all JSON mutation functions.

github-actions · 2026-04-13T07:23:41Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Bracket Notation Handling When stripping the `$.` prefix and the remaining input already contains bracket notation (e.g., `$.[]` becomes `[]`), the code then prepends `$.` again via `StringBuilder sb = new StringBuilder("$.")`, resulting in `$.[]`. This works correctly, but paths like `$[0]` (no dot after `$`) would become `[0]` and then `$.[0]`, which changes the semantics slightly. The test covers `$.[]` but not `$[0]` style paths. Verify that all valid JSONPath bracket-notation variants are handled correctly. if (input.startsWith("$.")) { input = input.substring(2); } else if (input.startsWith("$")) { input = input.substring(1); } if (input.isEmpty()) return "$"; StringBuilder sb = new StringBuilder("$."); Already-Converted Paths If a caller passes a path that has already been fully converted (e.g., `$.a[2].c`), the fix strips `$.` and re-processes `a[2].c` through the `StringBuilder` loop. The loop may not correctly handle bracket notation like `[2]` that is already in bracket form (not curly-brace form), potentially corrupting the path. Validate that pre-converted paths with mixed bracket/dot notation round-trip correctly through `convertToJsonPath`. if (input.startsWith("$.")) { input = input.substring(2); } else if (input.startsWith("$")) { input = input.substring(1); } if (input.isEmpty()) return "$"; StringBuilder sb = new StringBuilder("$.");

github-actions · 2026-04-13T07:24:07Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Verify correct argument type for delete eval call The test calls `JsonDeleteFunctionImpl.eval(...)` with a `String` as the second argument, but if the actual method signature expects a `List` or varargs of paths, this may cause a compilation error or incorrect behavior at runtime. Verify that the method signature of `JsonDeleteFunctionImpl.eval` matches the arguments being passed in the test. core/src/test/java/org/opensearch/sql/expression/json/JsonFunctionsTest.java [127-132] void test_jsonDeleteWithDollarPrefixedPath() throws Exception { // Issue #5167: json_delete with $.key path should remove the key Object result = - JsonDeleteFunctionImpl.eval("{\"name\":\"alice\",\"scores\":[90,85,92]}", "$.name"); + JsonDeleteFunctionImpl.eval("{\"name\":\"alice\",\"scores\":[90,85,92]}", List.of("$.name")); assertEquals("{\"scores\":[90,85,92]}", result); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid concern about whether `JsonDeleteFunctionImpl.eval` accepts a `String` or a `List` as the second argument. If the method signature expects a `List`, the test would fail to compile. However, the PR also adds a similar `JsonSetFunctionImpl.eval` call with a `String` argument that appears consistent, suggesting the API may accept strings directly.	Low
General	Handle edge case for `$[n]` style paths After stripping the `$.` prefix, if the remaining string starts with `[` (e.g., `$.[]` becomes `[``]`), prepending `$.` will produce `$.[*]` which is correct. However, if the input is `$.a[2].c`, stripping `$.` gives `a[2].c`, which is then processed by the loop and will produce `$.a[2].c` correctly. But if the input is `$[0]` (no dot after `$`), stripping `$` gives `[0]`, and the loop will prepend `$.` to produce `$.[0]`. This may be inconsistent with the original intent. Consider verifying that the stripping logic handles all edge cases like `$[0]` correctly, or add a test for this case. core/src/main/java/org/opensearch/sql/expression/function/jsonUDF/JsonUtils.java [27-35] if (input.startsWith("$.")) { input = input.substring(2); +} else if (input.startsWith("$[")) { + // e.g. "$[0]" -> "[0]", keep the bracket for the loop to handle + input = input.substring(1); } else if (input.startsWith("$")) { input = input.substring(1); } if (input.isEmpty()) return "$"; StringBuilder sb = new StringBuilder("$."); Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid edge case for `$[0]` style paths (without a dot after `$`), but the current code already handles this via the `else if (input.startsWith("$"))` branch, stripping just `$` and leaving `[0]`. The loop then prepends `$.` to produce `$.[0]`, which may or may not be the desired behavior. The suggestion adds a separate branch for `$[` but the behavior is identical to the existing `else if` branch, making the improvement marginal.	Low

songkant-aws · 2026-04-14T07:42:50Z

Nit: Add two more regression test cases for json_append and json_extend as the fix also covers fix in #5170

LantaoJin · 2026-04-14T07:49:11Z

+    if (input.startsWith("$.")) {
+      input = input.substring(2);
+    } else if (input.startsWith("$")) {
+      input = input.substring(1);
+    }


Could the input start with other invalid char? for example $$.name, etc.

This is the minimal change that handles all prefix variants (\$, \$., or none)

No, $$. is not standard JSONPath.

qianheng-aws added bug Something isn't working bugFix labels Apr 13, 2026

qianheng-aws marked this pull request as ready for review April 14, 2026 07:19

qianheng-aws requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, mengweieric, noCharger, penghuo, ps48, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners April 14, 2026 07:19

songkant-aws approved these changes Apr 14, 2026

View reviewed changes

LantaoJin reviewed Apr 14, 2026

View reviewed changes

LantaoJin approved these changes Apr 14, 2026

View reviewed changes

qianheng-aws merged commit 175d199 into opensearch-project:main Apr 14, 2026
42 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[BugFix] Fix json_set crash and json_delete no-op with $.key paths (#5167)#5339

[BugFix] Fix json_set crash and json_delete no-op with $.key paths (#5167)#5339
qianheng-aws merged 1 commit into
opensearch-project:mainfrom
qianheng-aws:worktree-agent-a9339de0

qianheng-aws commented Apr 13, 2026 •

edited

Loading

Uh oh!

qianheng-aws commented Apr 13, 2026

Uh oh!

github-actions Bot commented Apr 13, 2026

Uh oh!

github-actions Bot commented Apr 13, 2026

Uh oh!

songkant-aws commented Apr 14, 2026

Uh oh!

LantaoJin Apr 14, 2026

Uh oh!

qianheng-aws Apr 14, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

qianheng-aws commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

qianheng-aws commented Apr 13, 2026

Decision Log

Uh oh!

github-actions Bot commented Apr 13, 2026

PR Reviewer Guide 🔍

Uh oh!

github-actions Bot commented Apr 13, 2026

PR Code Suggestions ✨

Uh oh!

songkant-aws commented Apr 14, 2026

Uh oh!

LantaoJin Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qianheng-aws commented Apr 13, 2026 •

edited

Loading

qianheng-aws Apr 14, 2026 •

edited

Loading